Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: handleNewBooking #4 #15673

Merged
merged 20 commits into from
Oct 2, 2024
Merged

refactor: handleNewBooking #4 #15673

merged 20 commits into from
Oct 2, 2024

Conversation

Udit-takkar
Copy link
Contributor

@Udit-takkar Udit-takkar commented Jul 5, 2024

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Continuing refactor of handleNewBooking

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • N/A I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • N/A I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

@keithwillcode keithwillcode added consumer core area: core, team members only labels Jul 5, 2024
Copy link

vercel bot commented Jul 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ai ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 1:49pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 1:49pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 1:49pm

reqBodyRescheduleUid?: string;
};

export const checkBookingAndDurationLimits = async ({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check both booking and duration limits.

safeStringify({
users: users.map(getPiiFreeUser),
})
await validateBookingTimeIsNotOutOfBounds<typeof eventType>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved validation code

Comment on lines +172 to +178
const eventType = await getEventType({
eventTypeId: req.body.eventTypeId,
eventTypeSlug: req.body.eventTypeSlug,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just more simpler to read.

Comment on lines +39 to +48
export function getCustomInputsResponses(
reqBody: RequestBody,
eventTypeCustomInputs: getEventTypeResponse["customInputs"]
): NonNullable<CalendarEvent["customInputs"]> {
if (reqBody.customInputs && reqBody.customInputs.length > 0) {
return mapCustomInputs(reqBody.customInputs);
}

const responses = reqBody.responses || {};
return mapResponsesToCustomInputs(responses, eventTypeCustomInputs);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored this function

Comment on lines +30 to +37
return Object.entries(responses).reduce((acc, [fieldName, fieldValue]) => {
const foundInput = eventTypeCustomInputs.find((input) => slugify(input.label) === fieldName);
if (foundInput) {
acc[foundInput.label] = fieldValue;
}
return acc;
}, {} as NonNullable<CalendarEvent["customInputs"]>);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use .reduce method instead of for loop

throw new HttpError({ statusCode: 403, message: ErrorCode.CancelledBookingsCannotBeRescheduled });
}
}
let originalRescheduledBooking = originalBooking;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used let here because it originalRescheduledBooking is re assigned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this function is doing two things either getting the rescheduled booking or getting the seated booking. I think if we're going to abstract this logic it should be split into two functions that each havea single purpose.

@Udit-takkar Udit-takkar marked this pull request as ready for review July 10, 2024 20:48
@graphite-app graphite-app bot requested a review from a team July 10, 2024 20:49
@dosubot dosubot bot added bookings area: bookings, availability, timezones, double booking 💻 refactor labels Jul 10, 2024
Copy link

graphite-app bot commented Jul 10, 2024

Graphite Automations

"Add consumer team as reviewer" took an action on this PR • (07/10/24)

1 reviewer was added to this PR based on Keith Williams's automation.

"Add foundation team as reviewer" took an action on this PR • (09/19/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@Udit-takkar
Copy link
Contributor Author

To be merged after "booking with phone number"

@Udit-takkar
Copy link
Contributor Author

@joeauyeung fixed merge conflicts

Copy link
Contributor

@joeauyeung joeauyeung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving again after fixing merge conflicts

@CarinaWolli CarinaWolli merged commit b4f1b5a into main Oct 2, 2024
39 checks passed
@CarinaWolli CarinaWolli deleted the refactor/handleNewBooking-4 branch October 2, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bookings area: bookings, availability, timezones, double booking consumer core area: core, team members only enterprise area: enterprise, audit log, organisation, SAML, SSO ready-for-e2e 💻 refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants